Conversation
531b7b5 to
1eb8898
Compare
1eb8898 to
191ca3e
Compare
a6b27f9 to
c40a32d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
c40a32d to
e3ed19a
Compare
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
participant User as User
participant CLI as Status Command
participant Runtime as Docker Runtime
participant Config as Config
participant UI as Bubble Tea UI
participant Container as Container Status
participant AWS as AWS Emulator API
participant Sink as Output Sink
User->>CLI: Execute status command
CLI->>Config: Initialize config
Config-->>CLI: config ready
alt Interactive Mode
CLI->>UI: RunStatus(ctx, runtime, containers, host)
UI->>UI: NewApp()
UI->>UI: Start Bubble Tea program
UI->>Container: Start goroutine: Status(ctx, runtime, containers, host, sink)
par Container Status Fetching
Container->>Runtime: IsRunning(ctx, containerName)
Runtime-->>Container: running state
Container->>Runtime: ContainerStartedAt(ctx, containerName)
Runtime-->>Container: startup time
Container->>AWS: FetchVersion(ctx, host)
AWS-->>Container: version string
Container->>AWS: FetchResources(ctx, host)
AWS-->>Container: resources slice
Container->>Sink: Emit InstanceInfoEvent
Container->>Sink: Emit TableEvent
and UI Event Loop
UI->>UI: Process Tea messages
UI->>Sink: Append formatted events to display
end
Container-->>UI: runDoneMsg or runErrMsg
UI-->>User: Rendered status output
else Non-Interactive Mode
CLI->>Container: Status(ctx, runtime, containers, host, plainSink)
Container->>Runtime: IsRunning(ctx, containerName)
Runtime-->>Container: running state
Container->>Runtime: ContainerStartedAt(ctx, containerName)
Runtime-->>Container: startup time
Container->>AWS: FetchVersion(ctx, host)
AWS-->>Container: version string
Container->>AWS: FetchResources(ctx, host)
AWS-->>Container: resources slice
Container->>Sink: Emit events to stdout
Sink-->>User: Formatted status text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/output/plain_format_test.go (1)
114-227: Please add plain-sink parity coverage for these events.These assertions lock down
FormatEventLine, but the sink path also needs matching cases soNewPlainSinkcannot drift forInstanceInfoEventandTableEvent. I'd extendinternal/output/plain_sink_test.goas part of the same change.Based on learnings "Applies to internal/output/**/.go : When adding a new event type, update all of: internal/output/events.go (event type + Event union constraint + emit helper), internal/output/plain_format.go (line formatting fallback), and tests in internal/output/_test.go for formatter/sink behavior parity"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format_test.go` around lines 114 - 227, Add matching tests in internal/output/plain_sink_test.go to cover InstanceInfoEvent and TableEvent parity with FormatEventLine: update or add test cases that emit InstanceInfoEvent (both full and minimal) and TableEvent (with rows, empty) through NewPlainSink and assert the sink output and success/error behavior mirrors FormatEventLine results (use the same expected strings and wantOK semantics); ensure the tests call the sink's Emit/Write methods used by NewPlainSink and exercise narrow/wide terminal widths similar to TestFormatTableWidth so NewPlainSink cannot drift from FormatEventLine behavior.internal/container/status.go (1)
52-67: Inject the AWS emulator client instead of constructing it inStatus.Building
aws.NewClient(&http.Client{})inside the workflow hard-codes transport setup here and makes this path much harder to unit-test beyond the runtime mock. Bothcmd/status.goandinternal/ui/run_status.goare already the outer wiring layers, so this dependency fits better there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/status.go` around lines 52 - 67, The Status code hard-codes aws.NewClient(&http.Client{}) — change it to accept an injected AWS emulator client (use the existing aws client interface/type used by emulatorClient) instead of constructing one inside the switch; replace the inline creation and any direct references to aws.NewClient in the switch (emulatorClient.FetchVersion, emulatorClient.FetchResources) with the injected parameter/field (e.g., pass an aws client into the Status function or container struct), and update the outer wiring in the caller sites (cmd/status.go and internal/ui/run_status.go) to construct the real aws client and pass it in so unit tests can supply a mock client.internal/ui/app.go (1)
147-149: KeepbufferedLinesbounded too.
linesis capped viaappendLine, but these newPendingStop()branches append whole tables and other multi-line output straight intobufferedLines. A largestatustable can now grow UI state until the spinner drains. Please route buffered writes through the same trimming helper or capbufferedLinesseparately.Based on learnings, "Applies to internal/ui/**/*.go : Keep Bubble Tea message/history state bounded (for example, capped line buffer)".
Also applies to: 154-155, 230-232, 242-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 147 - 149, The PendingStop branches directly append multi-line output into bufferedLines (via a.spinner.PendingStop()), bypassing the existing capping logic in appendLine and allowing unbounded growth; change these branches to route every new line through the same trimming helper (i.e., call appendLine or a shared trimBufferedLines helper for each blank/line/blank piece) or apply a bounded-cap helper when concatenating tables so bufferedLines is trimmed to the same max length as lines; update all similar spots that directly append to bufferedLines (the other PendingStop/append occurrences) to use the same helper so UI message/history state stays bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/container/status.go`:
- Line 43: The call to endpoint.ResolveHost(c.Port, localStackHost) ignores its
error, which can leave host empty and cause misleading later failures; update
the status-checking code (around where ResolveHost is called and the host
variable is used) to capture the returned error, check it immediately, and
return or propagate a clear error when ResolveHost fails so we fail fast before
attempting to contact emulator endpoints or printing status like "is running
()". Ensure you reference ResolveHost and the host variable in your change and
handle the error path consistently with surrounding error handling.
In `@internal/emulator/aws/client.go`:
- Around line 71-73: The code in FetchResources uses bufio.NewScanner(resp.Body)
and then caps line size with scanner.Buffer(buf, 1024*1024), which limits NDJSON
lines to 1 MiB and can fail on large deployments; replace this by either using a
bufio.Reader to stream lines (e.g., read with ReadString('\n') or
ReadBytes('\n') from resp.Body) or raise/remove the ceiling on the scanner
(e.g., call scanner.Buffer(nil, <much larger size>) or allocate a larger buffer)
so that the scanner/reader can handle multi-megabyte NDJSON lines; update the
logic around the scanner variable and resp.Body handling in FetchResources to
use the chosen approach.
In `@internal/output/events.go`:
- Around line 60-75: Add typed helper functions EmitInstanceInfo and EmitTable
that wrap the generic Emit(...) so callers can send InstanceInfoEvent and
TableEvent via strongly-typed helpers; implement them mirroring the pattern used
by existing helpers (e.g., create func EmitInstanceInfo(ctx context.Context, e
InstanceInfoEvent) { Emit(ctx, e) } and func EmitTable(ctx context.Context, t
TableEvent) { Emit(ctx, t) }), and update any plain formatter and tests
(plain_format.go and internal/output/*_test.go) to include formatting/coverage
for InstanceInfoEvent and TableEvent to keep parity with other event types.
In `@internal/output/plain_format.go`:
- Around line 206-228: The current logic only shrinks widths[maxCol] down to a
floor (10) which still lets sum(widths)+overhead exceed totalWidth; change this
to ensure sum(widths)+overhead <= totalWidth by computing the excess =
(sum(widths) + overhead) - totalWidth and then reducing columns until excess <=
0; implement a loop that repeatedly picks the widest available column(s) (using
widths and maxCol selection or sort of indices) and decrements them down to a
reasonable per-column minimum (e.g., minWidth = 1 or 3) rather than a single
10-only floor, so that fixedWidth + sum(widths) is guaranteed not to exceed
totalWidth. Ensure you update widths in-place and stop when excess is
eliminated.
In `@internal/output/terminal.go`:
- Around line 9-15: The terminalWidth function currently queries both
os.Stdout.Fd() and os.Stderr.Fd(), which lets an interactive stderr drive width
when stdout is being redirected; change terminalWidth to consider only stdout
(use os.Stdout.Fd() with term.GetSize) and if stdout is not a TTY treat width as
unbounded/no-truncation (e.g., return a very large width or zero-as-unbounded)
so TableEvent output isn’t truncated when stdout is redirected; update
references in code that call terminalWidth accordingly.
---
Nitpick comments:
In `@internal/container/status.go`:
- Around line 52-67: The Status code hard-codes aws.NewClient(&http.Client{}) —
change it to accept an injected AWS emulator client (use the existing aws client
interface/type used by emulatorClient) instead of constructing one inside the
switch; replace the inline creation and any direct references to aws.NewClient
in the switch (emulatorClient.FetchVersion, emulatorClient.FetchResources) with
the injected parameter/field (e.g., pass an aws client into the Status function
or container struct), and update the outer wiring in the caller sites
(cmd/status.go and internal/ui/run_status.go) to construct the real aws client
and pass it in so unit tests can supply a mock client.
In `@internal/output/plain_format_test.go`:
- Around line 114-227: Add matching tests in internal/output/plain_sink_test.go
to cover InstanceInfoEvent and TableEvent parity with FormatEventLine: update or
add test cases that emit InstanceInfoEvent (both full and minimal) and
TableEvent (with rows, empty) through NewPlainSink and assert the sink output
and success/error behavior mirrors FormatEventLine results (use the same
expected strings and wantOK semantics); ensure the tests call the sink's
Emit/Write methods used by NewPlainSink and exercise narrow/wide terminal widths
similar to TestFormatTableWidth so NewPlainSink cannot drift from
FormatEventLine behavior.
In `@internal/ui/app.go`:
- Around line 147-149: The PendingStop branches directly append multi-line
output into bufferedLines (via a.spinner.PendingStop()), bypassing the existing
capping logic in appendLine and allowing unbounded growth; change these branches
to route every new line through the same trimming helper (i.e., call appendLine
or a shared trimBufferedLines helper for each blank/line/blank piece) or apply a
bounded-cap helper when concatenating tables so bufferedLines is trimmed to the
same max length as lines; update all similar spots that directly append to
bufferedLines (the other PendingStop/append occurrences) to use the same helper
so UI message/history state stays bounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 397cc057-ef0e-46af-a6b0-105babd2a502
📒 Files selected for processing (23)
CLAUDE.mdcmd/root.gocmd/status.gointernal/auth/mock_token_storage.gointernal/container/status.gointernal/container/status_test.gointernal/emulator/aws/aws.gointernal/emulator/aws/client.gointernal/emulator/aws/client_test.gointernal/emulator/aws/resource_name_test.gointernal/output/events.gointernal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/terminal.gointernal/runtime/docker.gointernal/runtime/mock_runtime.gointernal/runtime/runtime.gointernal/ui/app.gointernal/ui/components/input_prompt.gointernal/ui/components/message.gointernal/ui/run_status.gotest/integration/env/env.gotest/integration/status_test.go
| return output.NewSilentError(fmt.Errorf("%s is not running", name)) | ||
| } | ||
|
|
||
| host, _ := endpoint.ResolveHost(c.Port, localStackHost) |
There was a problem hiding this comment.
Handle ResolveHost failures explicitly.
Ignoring the error here can leave host empty and turns a local endpoint/config problem into a later fetch failure or is running () output. Fail fast before calling the emulator endpoints.
Suggested fix
- host, _ := endpoint.ResolveHost(c.Port, localStackHost)
+ host, err := endpoint.ResolveHost(c.Port, localStackHost)
+ if err != nil {
+ output.EmitSpinnerStop(sink)
+ return fmt.Errorf("resolve host for %s: %w", name, err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| host, _ := endpoint.ResolveHost(c.Port, localStackHost) | |
| host, err := endpoint.ResolveHost(c.Port, localStackHost) | |
| if err != nil { | |
| output.EmitSpinnerStop(sink) | |
| return fmt.Errorf("resolve host for %s: %w", name, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/container/status.go` at line 43, The call to
endpoint.ResolveHost(c.Port, localStackHost) ignores its error, which can leave
host empty and cause misleading later failures; update the status-checking code
(around where ResolveHost is called and the host variable is used) to capture
the returned error, check it immediately, and return or propagate a clear error
when ResolveHost fails so we fail fast before attempting to contact emulator
endpoints or printing status like "is running ()". Ensure you reference
ResolveHost and the host variable in your change and handle the error path
consistently with surrounding error handling.
| scanner := bufio.NewScanner(resp.Body) | ||
| buf := make([]byte, 1024*1024) | ||
| scanner.Buffer(buf, 1024*1024) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file in question
find . -name "client.go" -path "*/emulator/aws/*" -type fRepository: localstack/lstk
Length of output: 92
🏁 Script executed:
# Read the client.go file around lines 71-73
cat -n internal/emulator/aws/client.go | head -120 | tail -80Repository: localstack/lstk
Length of output: 2791
🏁 Script executed:
# Get the full context of the FetchResources function
cat -n internal/emulator/aws/client.goRepository: localstack/lstk
Length of output: 3800
Increase or remove the 1 MiB buffer ceiling on NDJSON resource lines.
scanner.Buffer(buf, 1024*1024) caps each scanned line at 1 MiB. Since each line contains all resources of a single AWS type, larger deployments can exceed this limit and cause FetchResources to fail. Use bufio.Reader directly or increase the buffer ceiling substantially to handle production deployments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/emulator/aws/client.go` around lines 71 - 73, The code in
FetchResources uses bufio.NewScanner(resp.Body) and then caps line size with
scanner.Buffer(buf, 1024*1024), which limits NDJSON lines to 1 MiB and can fail
on large deployments; replace this by either using a bufio.Reader to stream
lines (e.g., read with ReadString('\n') or ReadBytes('\n') from resp.Body) or
raise/remove the ceiling on the scanner (e.g., call scanner.Buffer(nil, <much
larger size>) or allocate a larger buffer) so that the scanner/reader can handle
multi-megabyte NDJSON lines; update the logic around the scanner variable and
resp.Body handling in FetchResources to use the chosen approach.
| type InstanceInfoEvent struct { | ||
| EmulatorName string | ||
| Version string | ||
| Host string | ||
| ContainerName string | ||
| Uptime time.Duration | ||
| } | ||
|
|
||
| type TableEvent struct { | ||
| Headers []string | ||
| Rows [][]string | ||
| } | ||
|
|
||
| type Event interface { | ||
| MessageEvent | AuthEvent | SpinnerEvent | ErrorEvent | ContainerStatusEvent | ProgressEvent | UserInputRequestEvent | LogLineEvent | ||
| MessageEvent | AuthEvent | SpinnerEvent | ErrorEvent | ContainerStatusEvent | ProgressEvent | UserInputRequestEvent | LogLineEvent | InstanceInfoEvent | TableEvent | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add typed emit helpers for the new event types.
InstanceInfoEvent and TableEvent are in the union now, but callers still have to use raw Emit(...) to send them. Please add EmitInstanceInfo and EmitTable alongside the existing helpers so domain code stays on the typed helper path.
As per coding guidelines "When adding a new event type, update all of: internal/output/events.go (event type + Event union constraint + emit helper), internal/output/plain_format.go (line formatting fallback), and tests in internal/output/*_test.go for formatter/sink behavior parity"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/output/events.go` around lines 60 - 75, Add typed helper functions
EmitInstanceInfo and EmitTable that wrap the generic Emit(...) so callers can
send InstanceInfoEvent and TableEvent via strongly-typed helpers; implement them
mirroring the pattern used by existing helpers (e.g., create func
EmitInstanceInfo(ctx context.Context, e InstanceInfoEvent) { Emit(ctx, e) } and
func EmitTable(ctx context.Context, t TableEvent) { Emit(ctx, t) }), and update
any plain formatter and tests (plain_format.go and internal/output/*_test.go) to
include formatting/coverage for InstanceInfoEvent and TableEvent to keep parity
with other event types.
| // Fixed overhead: 2 (indent) + (ncols-1)*2 (gaps between columns). | ||
| overhead := 2 + (ncols-1)*2 | ||
|
|
||
| // Find the widest column and let it absorb any overflow. | ||
| maxCol := 0 | ||
| for i := 1; i < ncols; i++ { | ||
| if widths[i] > widths[maxCol] { | ||
| maxCol = i | ||
| } | ||
| } | ||
| fixedWidth := overhead | ||
| for i, w := range widths { | ||
| if i != maxCol { | ||
| fixedWidth += w | ||
| } | ||
| } | ||
| maxFlexible := totalWidth - fixedWidth | ||
| if maxFlexible < 10 { | ||
| maxFlexible = 10 | ||
| } | ||
| if widths[maxCol] > maxFlexible { | ||
| widths[maxCol] = maxFlexible | ||
| } |
There was a problem hiding this comment.
The width cap still overflows narrow terminals.
Only the widest column is shrunk, and it is never allowed below 10 chars. If the other columns already consume more than totalWidth, the rendered row still wraps, so the truncation logic does not actually keep the table within the requested width.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/output/plain_format.go` around lines 206 - 228, The current logic
only shrinks widths[maxCol] down to a floor (10) which still lets
sum(widths)+overhead exceed totalWidth; change this to ensure
sum(widths)+overhead <= totalWidth by computing the excess = (sum(widths) +
overhead) - totalWidth and then reducing columns until excess <= 0; implement a
loop that repeatedly picks the widest available column(s) (using widths and
maxCol selection or sort of indices) and decrements them down to a reasonable
per-column minimum (e.g., minWidth = 1 or 3) rather than a single 10-only floor,
so that fixedWidth + sum(widths) is guaranteed not to exceed totalWidth. Ensure
you update widths in-place and stop when excess is eliminated.
| func terminalWidth() int { | ||
| for _, fd := range []uintptr{os.Stdout.Fd(), os.Stderr.Fd()} { | ||
| if w, _, err := term.GetSize(int(fd)); err == nil && w > 0 { | ||
| return w | ||
| } | ||
| } | ||
| return 80 |
There was a problem hiding this comment.
Don't truncate redirected output based on stderr's TTY width.
If stdout is piped but stderr is still interactive, this picks stderr's width and TableEvent output gets truncated even though the real sink is non-TTY. That means lstk status > out.txt can lose resource names and IDs. Prefer using stdout only here, or treat non-TTY stdout as unbounded/no-truncation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/output/terminal.go` around lines 9 - 15, The terminalWidth function
currently queries both os.Stdout.Fd() and os.Stderr.Fd(), which lets an
interactive stderr drive width when stdout is being redirected; change
terminalWidth to consider only stdout (use os.Stdout.Fd() with term.GetSize) and
if stdout is not a TTY treat width as unbounded/no-truncation (e.g., return a
very large width or zero-as-unbounded) so TableEvent output isn’t truncated when
stdout is redirected; update references in code that call terminalWidth
accordingly.
e3ed19a to
40a4944
Compare
Summary
Adds
lstk statuscommand that shows the status of running LocalStack emulators and their deployed AWS resources./_localstack/resourcesand displays them in a formatted tableInstanceInfoEventandTableEventoutput events that can be used by other commandsWith resources deployed
Emulator not started
No resources deployed